feat(marketplace): add curated upstream sources (closes #1136)#1145
feat(marketplace): add curated upstream sources (closes #1136)#1145sergio-sisternes-epam wants to merge 13 commits intomainfrom
Conversation
sergio-sisternes-epam
added
panel-review
There was a problem hiding this comment.
Pull request overview
Adds marketplace upstream authoring to APM so curators can expose selected plugins from external marketplaces, plus the supporting parser/resolver/cache/lockfile wiring and docs/tests around that flow.
Changes:
- Adds
marketplace.upstreams, upstream-sourcedpackages[]entries, and newapm marketplace upstream .../package add --upstream ...CLI paths. - Introduces upstream fetch/parse/resolve/cache/lockfile plumbing and fixes
apm packpackage counts for upstream-only builds. - Adds broad unit coverage and updates public/internal docs for the new authoring model.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
tests/unit/marketplace/test_yml_schema_upstreams.py |
Covers schema parsing and validation for upstream config. |
tests/unit/marketplace/test_yml_editor_upstreams.py |
Covers round-trip YAML editing helpers for upstream entries. |
tests/unit/marketplace/test_upstream_resolver.py |
Exercises upstream resolution precedence, diagnostics, and guards. |
tests/unit/marketplace/test_upstream_parser.py |
Covers strict upstream manifest parsing and rejection reasons. |
tests/unit/marketplace/test_upstream_cache.py |
Tests cache keys, integrity checks, and fetch/auth behavior. |
tests/unit/marketplace/test_lockfile_upstreams.py |
Verifies upstream provenance serialization into apm.lock.yaml. |
tests/unit/marketplace/test_builder_upstream_integration.py |
Tests builder output for mixed direct/upstream marketplace builds. |
tests/unit/commands/test_pack_upstream_count.py |
Pins the apm pack package-count regression fix. |
tests/unit/commands/test_marketplace_upstream.py |
Covers new upstream CLI commands and package add --upstream. |
src/apm_cli/marketplace/yml_schema.py |
Adds upstream dataclasses, parsing, and validation. |
src/apm_cli/marketplace/yml_editor.py |
Adds YAML editor helpers for upstream CRUD and package insertion. |
src/apm_cli/marketplace/upstream_resolver.py |
Implements upstream package resolution and diagnostics. |
src/apm_cli/marketplace/upstream_parser.py |
Adds strict parser for fetched upstream marketplace.json files. |
src/apm_cli/marketplace/upstream_cache.py |
Adds SHA-keyed cache and GitHub API fetch path for upstream manifests. |
src/apm_cli/marketplace/ref_resolver.py |
Exposes shared full-SHA regex for marketplace modules. |
src/apm_cli/deps/lockfile.py |
Adds upstream provenance types and YAML round-trip support. |
src/apm_cli/commands/pack.py |
Counts direct and upstream packages in pack output. |
src/apm_cli/commands/marketplace/upstream.py |
Adds upstream add/list/remove command group. |
src/apm_cli/commands/marketplace/plugin/add.py |
Extends package add flow for upstream-sourced packages. |
src/apm_cli/commands/marketplace/plugin/__init__.py |
Reuses shared SHA regex in marketplace plugin helpers. |
src/apm_cli/commands/marketplace/__init__.py |
Registers the new upstream command group. |
packages/apm-guide/.apm/skills/apm-usage/package-authoring.md |
Documents upstream schema and authoring examples. |
packages/apm-guide/.apm/skills/apm-usage/commands.md |
Adds internal command reference rows for upstream flows. |
docs/src/content/docs/reference/cli-commands.md |
Updates public CLI reference for upstream commands/options. |
docs/src/content/docs/guides/marketplace-upstreams.md |
Adds the new marketplace upstreams guide. |
docs/src/content/docs/enterprise/security.md |
Updates security/trust wording for curated upstream pass-through. |
docs/astro.config.mjs |
Adds sidebar navigation entry for the new guide. |
CHANGELOG.md |
Adds unreleased changelog entry for marketplace upstreams. |
| # -- assembly --------------------------------------------------------------- | ||
|
|
||
| def build(self) -> UpstreamResolver: | ||
| upstreams_by_alias = {u.alias: u for u in self._yml.upstreams} | ||
| # See ``MarketplaceBuilder._build_upstream_resolver`` for the | ||
| # rationale on ``canonical_full_name=None`` in v1. | ||
| return UpstreamResolver( | ||
| upstreams=upstreams_by_alias, | ||
| cache=UpstreamCache(), | ||
| ref_to_sha=self.ref_to_sha, | ||
| canonical_full_name=None, |
| # Claude Code uses ``type``; the lenient parser also tolerates an | ||
| # alternate ``source`` key inside the dict. Strict parsing requires | ||
| # ``type`` to be set explicitly so that ``unsupported-source-type`` | ||
| # rejections name the actual offending value. | ||
| source_type = raw.get("type", "") | ||
| if not isinstance(source_type, str) or not source_type: | ||
| return StrictRejection( | ||
| plugin_name=name, | ||
| reason="invalid-source-shape", | ||
| detail=f"plugin '{name}' source object missing 'type' discriminator", |
| if remote.name == f"refs/tags/{ref_or_branch}": | ||
| return remote.sha | ||
| if remote.name == f"refs/heads/{ref_or_branch}": | ||
| return remote.sha | ||
| raise BuildError(f"ref '{ref_or_branch}' not found in {owner}/{repo}") |
| # v1 host allow-list. Cross-host plugins (upstream on github, plugin on | ||
| # gitlab) are explicitly out of scope -- the plan reserves multi-host | ||
| # for v2 to avoid expanding the auth surface area in this slice. | ||
| _SUPPORTED_HOSTS: frozenset[str] = frozenset({"github.com"}) | ||
|
|
| _UPSTREAM_ALIAS_RE = re.compile(r"^[A-Za-z][A-Za-z0-9_-]*$") | ||
| _UPSTREAM_REPO_RE = re.compile(r"^[A-Za-z0-9._-]+/[A-Za-z0-9._-]+$") | ||
|
|
||
|
|
||
| def _validate_upstream_alias(alias: str) -> None: | ||
| if not _UPSTREAM_ALIAS_RE.match(alias): | ||
| raise MarketplaceYmlError( | ||
| f"Upstream alias '{alias}' must start with a letter and contain " | ||
| f"only letters, digits, '_' or '-'." |
| def _write_upstream_lockfile(self, upstream_resolved: list[ResolvedUpstreamPackage]) -> None: | ||
| """Persist upstream provenance into ``apm.lock.yaml``. | ||
|
|
||
| For every successfully resolved upstream package, record the | ||
| upstream marketplace coordinates (host/owner/repo/path/manifest | ||
| SHA) and the per-plugin pin (resolved SHA + emitted display | ||
| name + resolved source dict). The lockfile is the **only** | ||
| place provenance lives -- ``marketplace.json`` stays | ||
| Anthropic-conformant with no APM-specific keys. | ||
|
|
||
| Reads the existing lockfile (if any), preserves all unrelated | ||
| sections, replaces the ``upstreams`` block, and writes back. |
| ```bash | ||
| apm marketplace upstream add OWNER/REPO --alias ALIAS [OPTIONS] | ||
| apm marketplace upstream list [--verbose] | ||
| apm marketplace upstream remove ALIAS [--yes] |
| - **Marketplace upstreams: curated pass-through with allow-list governance.** Selectively re-expose plugins from an external marketplace (for example, a public Claude Code marketplace) under your own marketplace, with build-time commit pinning baked into `apm.lock.yaml`. | ||
| - New `apm marketplace upstream` CLI subgroup: `add` / `list` / `remove`. | ||
| - New `upstreams:` block in `apm.yml -> marketplace:`; `packages[]` entries pick between the existing `source:` shape and the new `upstream:` + `plugin:` shape (mutually exclusive per entry). | ||
| - Emitted `marketplace.json` is byte-for-byte Anthropic-conformant -- **no `metadata.apm.*` keys are injected**, so consumers see a vanilla plugin entry. | ||
| - APM does **not** re-host upstream content. Consumer installs always fetch plugin source from the upstream git host; air-gapped re-hosting is reserved for a future `distribution: rehost` mode. | ||
| - See the [Marketplace upstreams guide](https://microsoft.github.io/apm/guides/marketplace-upstreams/). (#1136) |
| if entry.ref is not None: | ||
| ref = entry.ref | ||
| sha = self._maybe_sha(ref) | ||
| return ref, sha, "curator-ref" |
| # 3. Upstream plugin's pin (sha preferred, else tag-style ref). | ||
| if plugin.source.sha is not None: | ||
| return plugin.source.sha, plugin.source.sha, "upstream-pin" | ||
| if plugin.source.ref is not None: | ||
| ref = plugin.source.ref | ||
| return ref, self._maybe_sha(ref), "upstream-pin" |
github-actions
Bot
removed
the
panel-review
APM Review Panel:
|
| Persona | B | R | N | Takeaway |
|---|---|---|---|---|
| Python Architect | 0 | 3 | 2 | Solid layered design with good DI and frozen dataclasses; three issues worth addressing: _REMOTE_SOURCE_RE duplicated 3x, _UpstreamResolverFactory back-references builder privates, and the rename guard is silently disabled in v1 without surfacing in security docs. |
| CLI Logging Expert | 0 | 1 | 2 | Output quality is solid overall; two nits on pluralisation and tree_item semantic misuse; one recommended on pack count transparency. |
| DevX UX Expert | 0 | 3 | 2 | CLI surface is solid and well-documented; two concrete issues: docs advertise --yes on upstream remove but it is unimplemented, and --plugin silently defaulting to --name creates a confusing footgun. |
| Supply Chain Security Expert | 1 | 1 | 1 | Repo-rename guard advertised as active but never wired in builder; cache integrity checks commit SHA not content bytes -- two supply-chain gaps worth addressing before wide rollout. |
| OSS Growth Hacker | 0 | 4 | 2 | Solid enterprise feature with a well-structured guide, but the quick-start breaks the 60-second runnable test, limitations are front-loaded, and the release hook is buried. |
| Auth Expert | 0 | 0 | 2 | Upstream fetch correctly delegates to AuthResolver.try_with_fallback with per-host credential scoping; one nit on unauth_first for private upstreams. |
| Doc Writer | 0 | 2 | 2 | New guide is well-structured and accurate; two planned-feature call-outs are missing per PROSE rules, and upstream add required-arg gap needs fixing in the CLI ref. |
| Test Coverage Expert | 1 | 2 | 0 | 165 unit tests present and correct; all cross-module upstream flows (build->lockfile, CLI->disk) lack integration-with-fixtures tier coverage per surface floor. |
B = blocking-severity findings, R = recommended, N = nits.
Counts are signal strength, not gates. The maintainer ships.
Top 5 follow-ups
- [Supply Chain Security Expert] (blocking-severity) Wire canonical_full_name into the rename/takeover guard or update the trust table to 'NO (deferred to v2)' -- The trust table makes an affirmative security claim ('Repo-rename / takeover guard: YES') that the code falsifies. This is the only finding that is both a correctness issue and a documentation deception -- it must match reality before merge.
- [Test Coverage Expert] (blocking-severity) Add integration-with-fixtures test covering upstream add -> build -> lockfile write end-to-end against real fixture files -- The entire cross-module upstream build pipeline is covered only by MagicMock unit tests. outcome:missing on a governed-by-policy + secure-by-default surface inherits blocking-tier weight; silent drift in resolver-to-lockfile handoff would go undetected.
- [DevX UX Expert] Either implement --yes on
upstream removeor remove it from all doc locations -- docs advertise [--yes] but the option does not exist; users who copy-paste the synopsis get a runtime error. The peer command already has the flag, so user expectation is set. - [DevX UX Expert] Remove the silent --plugin default to --name when --upstream is set; require --plugin explicitly -- Silent default causes a cryptic 'plugin not found in upstream' error when a curator writes --name as a local alias. The footgun is invisible at the call site.
- [Supply Chain Security Expert] Add content_sha256 to upstream cache meta.json and verify on every get() -- Current integrity check confirms the right commit SHA index but does not verify the fetched manifest bytes; tampered cache content passes the check. Adding a content hash closes the gap with minimal implementation cost.
Architecture
classDiagram
direction LR
class MarketplaceBuilder {
<<Facade>>
+resolve() ResolveResult
+build() BuildReport
+compose_marketplace_json() dict
-_build_upstream_resolver(yml) UpstreamResolver
}
class _UpstreamResolverFactory {
<<Factory>>
-_builder MarketplaceBuilder
-_host_resolvers dict
+ref_to_sha(host,owner,repo,ref) str
+version_range(host,owner,repo,range) tuple
+build() UpstreamResolver
}
class UpstreamResolver {
<<Facade>>
-_upstreams dict
-_cache UpstreamCache
-_ref_to_sha RefToShaResolver
-_canonical_full_name CanonicalFullNameResolver
-_version_range_resolver VersionRangeResolver
-_fetched dict
-_diagnostics list
+resolve_all(entries) tuple
+resolve_package(entry) ResolvedUpstreamPackage
-_get_or_fetch_upstream(upstream) _UpstreamFetchRecord
-_apply_precedence_ladder(entry,plugin,upstream) tuple
}
class UpstreamCache {
<<IOBoundary>>
-_base_dir Path
-_fetch_callback Callable
+get(key) dict
+put(key, manifest) None
+get_or_fetch(key) dict
}
class UpstreamCacheKey {
<<ValueObject>>
+host str
+owner str
+repo str
+sha str
+path str
+composite str
+fingerprint str
+directory_name str
}
class ResolvedUpstreamPackage {
<<ValueObject>>
+entry UpstreamPackageEntry
+upstream Upstream
+plugin StrictPlugin
+plugin_sha str
+upstream_manifest_sha str
+pin_source str
}
class UpstreamResolverDiagnostic {
<<ValueObject>>
+level str
+code str
+message str
+upstream_alias str
}
class _UpstreamFetchRecord {
+manifest StrictManifest
+manifest_sha str
+canonical_full_name str
}
class Upstream {
<<ValueObject>>
+alias str
+repo str
+ref str
+branch str
+host str
+allow_head bool
}
class UpstreamPackageEntry {
<<ValueObject>>
+name str
+upstream_alias str
+plugin str
+ref str
+version str
}
class StrictManifest {
+plugins list
+rejections list
+find_plugin(name) StrictPlugin
}
class StrictPlugin {
<<ValueObject>>
+name str
+source StrictPluginSource
}
class LockedUpstream {
+alias str
+manifest_sha str
+plugins dict
+to_dict() dict
+from_dict() LockedUpstream
}
class LockFile {
+upstreams dict
+to_yaml() str
+from_yaml() LockFile
}
class _UpstreamResolverFactory:::touched
class UpstreamResolver:::touched
class UpstreamCache:::touched
class UpstreamCacheKey:::touched
class ResolvedUpstreamPackage:::touched
class UpstreamResolverDiagnostic:::touched
class _UpstreamFetchRecord:::touched
class Upstream:::touched
class UpstreamPackageEntry:::touched
class StrictManifest:::touched
class StrictPlugin:::touched
class LockedUpstream:::touched
classDef touched fill:#fff3b0,stroke:#d47600
MarketplaceBuilder *-- _UpstreamResolverFactory : creates
_UpstreamResolverFactory ..> UpstreamResolver : assembles
UpstreamResolver *-- UpstreamCache : cache
UpstreamResolver ..> UpstreamPackageEntry : resolves
UpstreamResolver ..> Upstream : reads registration
UpstreamResolver ..> _UpstreamFetchRecord : stores
UpstreamResolver ..> ResolvedUpstreamPackage : returns
UpstreamResolver ..> UpstreamResolverDiagnostic : emits
_UpstreamFetchRecord *-- StrictManifest : holds
StrictManifest *-- StrictPlugin : contains
UpstreamCache ..> UpstreamCacheKey : keyed by
LockFile *-- LockedUpstream : stores
MarketplaceBuilder ..> Upstream : loads from apm.yml
MarketplaceBuilder ..> UpstreamPackageEntry : loads from apm.yml
note for _UpstreamResolverFactory "Factory: builds UpstreamResolver with injected Strategy callables for ref_to_sha / version_range"
note for UpstreamCache "Content-addressed by 40-char git SHA. Defence-in-depth integrity re-check on every hit."
note for UpstreamResolver "Collect-then-render: accumulates UpstreamResolverDiagnostic list; builder lifts to BuildDiagnostic rows"
flowchart TD
CLI["apm marketplace build"] --> MB["MarketplaceBuilder.build()\nbuilder.py"]
MB --> LY["[I/O] _load_yml()\nload_marketplace_from_apm_yml()"] --> MYml["MarketplaceYml\n(packages, upstreams)"]
MYml --> Partition{"resolve()\npartition by isinstance"}
Partition -->|DirectPackageEntry| Direct["ThreadPoolExecutor\n_resolve_entry() x N\nbuilder.py"]
Direct --> LSR["[NET] RefResolver.list_remote_refs()\ngit ls-remote"]
LSR --> RP["ResolvedPackage list"]
Partition -->|UpstreamPackageEntry| URF["_UpstreamResolverFactory.build()\nbuilder.py"]
URF --> UR["UpstreamResolver.resolve_all()\nupstream_resolver.py"]
UR --> GOFT["_get_or_fetch_upstream(upstream)\nupstream_resolver.py"]
GOFT --> Fetched{"alias in _fetched?"}
Fetched -->|hit| FetchRecord
Fetched -->|miss| Rename["[NET] canonical_full_name()\n(None in v1 -- guard skipped)"]
Rename --> RefSHA["[NET] ref_to_sha()\nRefResolver.list_remote_refs()"]
RefSHA --> CK["compute_cache_key()\nupstream_cache.py"]
CK --> UCGet["[FS] UpstreamCache.get(key)\nread manifest.json + meta.json"]
UCGet --> CacheHit{"integrity valid?"}
CacheHit -->|yes| Raw
CacheHit -->|no / miss| Fetch["[NET] _default_fetch_via_github_api()\nGitHub Contents API\nupstream_cache.py"]
Fetch --> UCPut["[FS] UpstreamCache.put(key)\nwrite manifest.json then meta.json"]
UCPut --> Raw["raw manifest dict"]
Raw --> Parse["parse_marketplace_strict()\nupstream_parser.py"]
Parse --> FetchRecord["_UpstreamFetchRecord\n(manifest, sha, canonical)"]
FetchRecord --> Ladder["_apply_precedence_ladder()\nupstream_resolver.py\ncurator-ref > version > upstream-pin > same-repo-sha > head"]
Ladder --> RUP["ResolvedUpstreamPackage"]
RP --> Compose["compose_marketplace_json()\nbuilder.py"]
RUP --> Compose
Compose --> Output["[FS] atomic_write()\nmarketplace.json"]
RUP --> Lock["[FS] LockFile.upstreams\napm.lock.yaml"]
Recommendation
Address the two blocking-tier items before merge: (1) align the trust table with the actual behavior of the rename guard -- one line of code or one line of docs, not both claiming opposite things; (2) add at least one integration-with-fixtures test that runs the full upstream build -> lockfile write pipeline against real fixture files on disk. All other panel findings -- --yes on upstream remove, --plugin footgun, cache content hash, doc planned-feature callouts, and the CHANGELOG/guide reframe -- are strong recommended improvements but do not gate merge. Once the two blocking items are resolved, this PR is a clean ship: the architecture is sound, the test count is substantial, and the feature is the strongest enterprise positioning surface APM has delivered.
Full per-persona findings
Python Architect
-
[recommended] _REMOTE_SOURCE_RE regex duplicated verbatim across 3 modules at
src/apm_cli/marketplace/upstream_cache.py:107
upstream_cache.py:107, upstream_parser.py:87, and yml_schema.py:80 each define the identical regex compile call. The PR already consolidated FULL_SHA_RE into ref_resolver -- the same treatment should apply here. A divergence between copies would introduce a silent validation gap in one layer while the others still reject.
Suggested: Move REMOTE_SOURCE_RE into ref_resolver.py alongside FULL_SHA_RE and import it in all three consumers. -
[recommended] _UpstreamResolverFactory holds a back-reference to MarketplaceBuilder and calls private methods at
src/apm_cli/marketplace/builder.py:238
_UpstreamResolverFactory.init stores self._builder: MarketplaceBuilder and reaches into self._builder._host, self._builder._options, self._builder._get_resolver(), and self._builder._auth_resolver. This defeats the stated purpose of extracting the factory to improve readability and testability.
Suggested: Pass specific values at construction time: init(self, *, options, host, get_resolver, auth_resolver, upstreams). The builder assembles the kwargs and passes them; the factory is then independently testable. -
[recommended] Repo-rename guard is advertised as active but wired as canonical_full_name=None in v1 at
src/apm_cli/marketplace/builder.py:332
The PR body, security docs, and UpstreamResolver docstring all describe the repo-rename guard as a supply-chain protection. In practice, _UpstreamResolverFactory.build() passes canonical_full_name=None, which causes _get_or_fetch_upstream to skip the guard entirely. The security documentation creates a false expectation.
Suggested: Either (a) add warning log when canonical_full_name is None so operators can see it is skipped, or (b) update security doc and UpstreamResolver docstring to state 'rename guard deferred to v2'. -
[nit] UpstreamPackageEntry.upstream_alias attribute name diverges from the YAML key 'upstream' at
src/apm_cli/marketplace/yml_schema.py:349
The YAML key is 'upstream', the dataclass field is 'upstream_alias', and the loader must translate between them. Adds friction at every call site. -
[nit] yml_editor.py carries unused 'from typing import List, Optional' imports at
src/apm_cli/marketplace/yml_editor.py:19
Line 19 imports List and Optional with noqa: UP035 comment, but neither symbol is used anywhere in the file.
Suggested: Remove the List, Optional import and the noqa comment.
CLI Logging Expert
-
[recommended] pack.py success message silently merges direct + upstream counts with no breakdown at
src/apm_cli/commands/pack.py:219
A curator who adds 3 direct packages and 12 upstream packages sees '15 package(s)' with no signal that upstreams are even involved. When upstream_resolved is non-empty a parenthetical breakdown makes the output self-explanatory.
Suggested: When upstream_resolved is non-empty emit: 'Built marketplace.json (N direct + M upstream = T package(s)) -> path'. Apply the same pattern to the dry_run_notice line. -
[nit] tree_item is semantically wrong for upstream list rows at
src/apm_cli/commands/marketplace/upstream.py:193
tree_item is documented as 'Log a tree sub-item' and renders unconditionally in green. In list_cmd the upstream entries are primary content, not package-tree children. If a future reviewer adds a verbose guard to tree_item the list will silently break.
Suggested: Replace with logger.info per entry or introduce a dedicated logger.list_item helper. -
[nit] Awkward (s) pluralisation in list_cmd count header at
src/apm_cli/commands/marketplace/upstream.py:185
"N upstream(s) registered:" is the only place in the codebase using the (s) anti-pattern.
Suggested:noun = 'upstream' if len(entries) == 1 else 'upstreams'; logger.info(f'{len(entries)} {noun} registered:', symbol='info')
DevX UX Expert
-
[recommended] docs advertise
upstream remove ALIAS [--yes]but --yes is not implemented atdocs/src/content/docs/reference/cli-commands.md:245
cli-commands.md shows [--yes] in synopsis. User who copy-pastes the documented example gets 'No such option: --yes'. The peer commandapm marketplace package removehas --yes, so users will expect it here too.
Suggested: Either implement --yes on upstream remove, or remove it from all doc locations and add a note that the command self-guards against referenced aliases. -
[recommended] --plugin silently defaults to --name, creating a confusing footgun when --name is a curator alias at
src/apm_cli/commands/marketplace/plugin/add.py:408
If a curator writesapm marketplace package add --upstream gitnexus --name acme-gitnexus(omitting --plugin), APM looks for a plugin called 'acme-gitnexus' in the upstream. The error ('plugin not found in upstream') will be cryptic because the user never mentioned the upstream plugin name.
Suggested: Remove the silent default: require --plugin explicitly when --upstream is set (raise UsageError if both are absent). -
[recommended] apm pack success line hides upstream contribution, making the count opaque at
src/apm_cli/commands/pack.py:221
A curator cannot tell from output whether upstreams resolved correctly or were silently skipped.
Suggested: When upstream_resolved is non-empty: 'Built marketplace.json (N direct + M upstream = T package(s)) -> path'. -
[nit]
upstream adduses --alias as a required option rather than positional argument, breaking the git-remote-add mental model atsrc/apm_cli/commands/marketplace/upstream.py:619
git remote add <name> <url>uses positional args for primary identifiers.apm marketplace upstream add owner/repo --alias nameforces an extra flag for something semantically primary. -
[nit]
upstream addrequires --ref OR --branch with no default, unlike every other apm add command atsrc/apm_cli/commands/marketplace/upstream.py:644
Intentional for safety but the asymmetry will surprise users who follow the pattern frompackage add.
Suggested: Add a one-liner to the error message explaining why there is no auto-pin default.
Supply Chain Security Expert
-
[blocking] Rename/takeover guard is always disabled: builder passes canonical_full_name=None at
src/apm_cli/marketplace/builder.py:332
The PR trust-model table explicitly states 'Repo-rename / takeover guard: YES'. However _UpstreamResolverFactory.build() hard-codes canonical_full_name=None, meaning the guard is never reachable. The lockfile records curator-typed canonical_full_name -- not GitHub-reported name -- defeating the guard. A renamed or account-takeover repo silently resolves without warning.
Suggested: Wire in a real canonical_full_name callable. If v1 deferral intended, document as 'NO (deferred to v2)' in trust table and remove the rename-guard machinery so it cannot mislead auditors. -
[recommended] Cache integrity verifies commit SHA tag but not content hash of fetched bytes at
src/apm_cli/marketplace/upstream_cache.py:324
upstream_cache.py get() compares meta['sha'] to key.sha. This confirms indexing to the right commit but does NOT verify the manifest.json bytes hash. Tampered cache content passes the integrity check.
Suggested: In put(), compute sha256 over manifest bytes and store as content_sha256 in meta.json; verify on every get(). -
[nit] Generic except clause may surface raw exception strings including response body fragments at
src/apm_cli/marketplace/upstream_cache.py:496
bare 'except Exception as exc: raise UpstreamCacheError(...{exc}...)' includes str(exc) verbatim. For some network libraries this can embed response body fragments.
Suggested: Replace with type(exc).name or a pre-scrubbed message.
OSS Growth Hacker
-
[recommended] Quick start requires the reader to run a preflight command -- breaks the 60-second runnable test at
docs/src/content/docs/guides/marketplace-upstreams.md
The example SHA placeholder forces stop, open terminal, run git ls-remote, copy, return to guide. Doubles time-to-first-success; Why do we need a GitHub token? #1 drop-off point for docs conversion.
Suggested: Embed actual resolved SHA inline in example and add comment:# update this SHA before committing by running: git ls-remote ... HEAD -
[recommended] Limitations listed prominently after failure modes -- enterprise evaluators will bounce at
docs/src/content/docs/guides/marketplace-upstreams.md
The 'What is NOT supported in v1' section appears after the failure-mode error blocks. Stacking negatives trains readers to associate this feature with what it cannot do.
Suggested: Rename to 'Roadmap / known gaps', move to bottom, reframe bullets as forward-looking. -
[recommended] No elevator-pitch hook -- guide opens with tool analogy, not user outcome at
docs/src/content/docs/guides/marketplace-upstreams.md
An enterprise champion needs a sentence to paste into Slack. Hook = conversion.
Suggested: Prepend: 'Ship vetted AI capabilities company-wide -- without maintaining forks.' -
[recommended] CHANGELOG entry lacks a tweet-sized release hook at
CHANGELOG.md
The CHANGELOG is raw material for release posts. Entry is accurate but reads like a spec, not a story.
Suggested: Open CHANGELOG entry with a bolded story sentence before the bullets. -
[nit] Quick-start example references a real third-party repo -- tutorial breaks if repo renames at
docs/src/content/docs/guides/marketplace-upstreams.md
abhigyanpatwari/GitNexus is a live external resource. If renamed, triggers APM's own rename guard in the tutorial. -
[nit] Guide sidebar has no cross-link from the marketplace-authoring guide at
docs/src/content/docs/guides/marketplace-authoring.md
A curator who reads marketplace-authoring to completion will never learn upstreams exist.
Auth Expert
-
[nit] try_with_fallback called with unauth_first=True for all upstream hosts including private ones at
src/apm_cli/marketplace/upstream_cache.py
Correct for public upstreams but private upstream repos will always incur an extra 404 round-trip before auth retry. Consider deriving the flag from host_info.has_public_repos or documenting the limitation. -
[nit] Private upstream repos cannot resolve non-SHA refs in v1 at
src/apm_cli/marketplace/builder.py
_resolver_for_host returns token=None for upstream hosts differing from the curator host. A changelog note would help curators trying to use private upstream repos.
Doc Writer
-
[recommended] upstream add: --ref or --branch is required but docs list both as optional at
docs/src/content/docs/guides/marketplace-upstreams.md
The code raises UsageError when neither is provided. Both are listed as optional with no indication at least one is mandatory. User who omits both gets runtime error with no doc warning.
Suggested: In the options table annotate: 'One of --ref or --branch is required. They are mutually exclusive.' Mirror this in cli-commands.md. -
[recommended] Two planned features mentioned inline without :::note[Planned] callouts at
docs/src/content/docs/guides/marketplace-upstreams.md
Persona rules require planned features use Starlight callouts. Two forward references appear as prose: 'future distribution: rehost mode' and 'future apm marketplace upstream refresh'.
Suggested: Replace inline parentheticals with:::note[Planned]Starlight callouts. -
[nit] Quick Start uses real third-party repo and real commit SHA at
docs/src/content/docs/guides/marketplace-upstreams.md
If abhigyanpatwari/GitNexus is renamed/deleted, example silently breaks. cli-commands.md correctly uses placeholder SHAs.
Suggested: Replace real SHA with a clearly fictitious placeholder (e.g. abcdef1234567890abcdef1234567890abcdef12). -
[nit] Nav entry label casing differs from guide frontmatter title at
docs/astro.config.mjs
Nav uses 'Marketplace Upstreams' (title case); frontmatter uses 'Marketplace upstreams' (sentence case).
Test Coverage Expert
-
[blocking] Full upstream build pipeline has zero integration-with-fixtures tests; only unit-level mocks exist at
tests/integration/marketplace/test_build_integration.py
The entire upstream add -> build -> lockfile write flow is covered only by unit tests where UpstreamCache.fetch_callback is always MagicMock. tests/integration/marketplace/ contains zero 'upstream' references. The cross-module surface (upstream_parser -> upstream_resolver -> upstream_cache -> builder -> lockfile) requires integration-with-fixtures coverage per tier floor.
Proof (missing at tests/integration/marketplace/test_build_integration.py::test_upstream_build_writes_lockfile_provenance):assert 'upstreams' in yaml.safe_load(lockfile_path.read_text()); assert lock['upstreams']['gitnexus']['manifest_sha'] == expected_sha-- proves: Running apm marketplace build with an upstream entry writes correct provenance to apm.lock.yaml [governed-by-policy,portability-by-manifest] -
[recommended] apm marketplace upstream CLI tested only via CliRunner (unit tier); no subprocess-level integration test at
tests/unit/commands/test_marketplace_upstream.py
CliRunner runs in-process without subprocess isolation, real CWD pivoting, or binary exit-code capture. No test in tests/integration/ invokes 'apm marketplace upstream' via subprocess.
Proof (passed at tests/unit/commands/test_marketplace_upstream.py::TestUpstreamAdd.test_branch_requires_allow_head):assert result.exit_code != 0; assert 'allow-head' in result.output.lower()-- proves: apm marketplace upstream add rejects a branch ref without --allow-head [devx,governed-by-policy] -
[recommended] Lockfile upstream provenance round-trip tested at unit tier only at
tests/unit/marketplace/test_lockfile_upstreams.py
All 7 lockfile test invocations use MagicMock fetch_callback. A snapshot-fixture approach (canned manifest.json on disk, real UpstreamCache.get/put cycle) would certify the full read+write+diff contract without live network.
Proof (passed at tests/unit/marketplace/test_lockfile_upstreams.py::TestLockFileUpstreams.test_yaml_round_trip_preserves_upstreams):restored = LockFile.from_yaml(yaml_str); assert restored.upstreams['gitnexus'].manifest_sha == SHA_UPSTREAM_MANIFEST-- proves: apm.lock.yaml upstream provenance survives YAML serialize/deserialize round-trip [governed-by-policy,secure-by-default]
This panel is advisory. It does not block merge. Re-apply the
panel-review label after addressing feedback to re-run.
Generated by PR Review Panel for issue #1145 · ● 4.2M · ◷
Wave C (Recommended):
- C1: add --yes/-y flag to 'upstream remove' with pre-validation before prompt
- C2: require explicit --plugin when --upstream is set (remove silent default)
- C3: pack success line shows 'N direct + M upstream' breakdown when upstreams > 0
- C4: consolidate _REMOTE_SOURCE_RE into OWNER_REPO_RE in ref_resolver.py;
import in upstream_cache, upstream_parser, and yml_schema (DRY)
- C5: add content_sha256 to UpstreamCache meta.json; verify on cache hit
- C6-C8: docs -- move trust model to bottom of marketplace-upstreams.md,
add :::note[Planned] callouts for refresh and apm doctor,
replace live SHA with fictitious placeholder,
add cross-link from marketplace-authoring.md to upstreams guide
Wave D (Nits):
- D1: replace tree_item() with logger.info() for flat list rows
- D2: fix (s) pluralisation -- '1 upstream' / '2 upstreams'
- D3: remove unused List/Optional imports from yml_editor.py
- D4: nav label 'Marketplace Upstreams' -> 'Marketplace upstreams' (sentence-case)
- D5: scrub str(exc) -> type(exc).__name__ in upstream_cache.py log messages
Test updates: fix TestUpstreamRemove tests to pass --yes, update alias to
-bad-alias (digit-leading now valid per B5), update pack count test assertions
to match new 'N direct + M upstream' format.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Response to Copilot and APM Panel feedbackThank you both for the thorough review -- the panel's two-blocking-item framing was exactly right, and Copilot's inline comments provided precise line-level guidance. All 10 Copilot inline comments and all panel findings (blocking, recommended, nits) have been addressed across four commits on this branch. Copilot inline comments1. 2. 3. 4. 5. 6. 7. 8. 9. 10. APM Panel findingsBlocking (both resolved)[1] Supply Chain Security -- trust table claims rename guard active, code falsifies it [2] Test Coverage -- entire upstream build -> lockfile write pipeline has no integration-with-fixtures test
Registered in Recommended (all resolved in
|
| Check | Status |
|---|---|
| Unit tests | 7,551 passed |
| New integration tests | 3 passed (test_upstream_build_integration.py) |
Lint (ruff check + ruff format --check) |
Clean |
| Copilot inline comments | 10/10 addressed |
| Panel blocking findings | 2/2 resolved |
| Panel recommended findings | 5/5 resolved |
| Panel nits | 5/5 resolved |
Ready for a second review pass.
Introduce the type-level discriminated union for marketplace authoring: - New 'Upstream' dataclass for marketplace.upstreams[] entries (alias, repo, path, ref, branch, host, allow_head); reproducibility guard rejects missing ref unless allow_head is opted in. - New 'UpstreamPackageEntry' dataclass for upstream-sourced packages[] entries (name, upstream_alias, plugin, optional curator overrides). - 'PackageEntry' (the existing direct shape) unchanged; new public alias 'DirectPackageEntry' added for explicit naming, plus 'MarketplacePackage' union alias. - Strict per-shape key sets (_DIRECT_PACKAGE_KEYS, _UPSTREAM_PACKAGE_KEYS, _UPSTREAM_REGISTRATION_KEYS) catch typos in the wrong shape at parse time. - _parse_package_entry now dispatches by shape (presence of source vs upstream); both shapes can coexist in the same packages[] list. - Cross-validation: every upstream-sourced entry references a declared upstreams[] alias; cross-shape package name uniqueness enforced (prevents dependency-confusion / shadowing); duplicate (upstream_alias, plugin) pairs rejected. - Anthropic-conformant emission contract preserved: this commit adds no marketplace.json output keys. 23 new unit tests cover both shapes, mutex validation, alias/host/repo syntax, path traversal, cross-validation, and the additive guarantee for existing curators. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
New module 'marketplace/upstream_parser.py' is the curator-side
counterpart to the lenient 'parse_marketplace_json'. Where the lenient
parser silently drops unrecognised entries (correct for consumer
browse), the strict parser returns named per-entry rejections so
curators see exactly which upstream plugin was refused and why.
- Public API: 'parse_marketplace_strict(data, *, upstream_owner_repo,
upstream_host)' returning a 'StrictManifest' with both accepted
'plugins' (StrictPlugin) and 'rejections' (StrictRejection).
- Closed catalogue 'REJECTION_REASONS' enumerates every named reason
('missing-source', 'invalid-repo', 'npm-source',
'unsupported-source-type', 'unsupported-host', 'path-traversal',
'invalid-ref', 'invalid-sha', 'unknown-source-key',
'unknown-plugin-key', 'duplicate-name', etc.) so CLI/builder
diagnostics can pattern-match deterministically.
- Source-shape support matrix (v1):
* 'repository: owner/repo' (Copilot CLI shape).
* 'source: {type: github, repo, ref, sha}' (Claude Code).
* 'source: {type: git-subdir, repo, path, ref, sha}'.
* 'source: "./foo"' / '"foo"' string-shorthand resolved as a
git-subdir of the upstream marketplace's own repo, gated by
'metadata.pluginRoot'. Routes through 'path_security.
validate_path_segments' so '..' / '%2e%2e' / absolute paths
are rejected.
- All non-github hosts and 'npm' / 'tarball' / arbitrary URL sources
are explicit rejections with named reasons (no silent skip).
- Strict 'find_plugin' is case-sensitive (vs. the lenient
case-insensitive lookup) so curator typos in 'plugin: <name>'
fail loudly.
42 new tests in 'test_upstream_parser.py' cover every supported source
shape, every named rejection reason, partial-success multi-entry
behaviour, duplicate-name detection, and a real-world fixture mirroring
'abhigyanpatwari/GitNexus/marketplace.json'.
Smoke-tested live against the GitNexus upstream:
plugins: [('gitnexus', 'git-subdir', 'abhigyanpatwari/GitNexus',
'gitnexus-claude-plugin')]
rejections: ()
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Introduces ``apm_cli.marketplace.upstream_cache`` -- the curator-side cache used by the upcoming ``UpstreamResolver`` when building a marketplace whose packages are sourced from an external marketplace. Design contract - Content-addressed by manifest SHA; no TTL, immutable per SHA. - Windows-safe key: ``__`` delimiter (not ``::``); composite is SHA-256-hashed and hex-truncated for filesystem safety. - Defence-in-depth: rejects ``__`` in inputs; ``validate_path_segments`` on path inputs; integrity check on every cache hit (recorded SHA must match requested SHA, else miss). - Per-upstream-host auth via ``AuthResolver.try_with_fallback`` with ``unauth_first=True`` to avoid attaching the curator's PAT to public upstream repos (supply-chain panel item 3). - ``classify_host`` guard refuses non-GitHub hosts in v1, mirroring the strict parser's allow-list. - Offline mode: ``get_or_fetch(..., offline=True)`` raises a named error on miss instead of touching the network. Tests (44 new, all passing) - Cache-key validation: Windows safety, hash stability, delimiter rejection, SHA shape, host shape, path traversal. - Read/write: hit, miss, integrity mismatch -> miss, corrupt manifest/meta -> miss, separate dirs per key. - get_or_fetch: hit skips fetch, miss fetches+caches, offline raises. - Default fetch callback: classify-host guard refuses gitlab.com, unauth_first path attaches no Authorization header, auth path attaches token only on GitHub-family hosts, 404 surfaces as named error. Lint: ``ruff check`` and ``ruff format --check`` both silent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Introduces apm_cli.marketplace.upstream_resolver -- the layer between the schema dataclasses and MarketplaceBuilder. It owns three invariants: 1. Atomic-fetch: each registered Upstream is fetched + strict-parsed at most once per build, even when many packages reference the same upstream alias. 2. Repo-rename guard: compares API-reported canonical full_name against the configured upstream.repo (case-insensitive) and fails closed on mismatch. 3. Precedence ladder: curator-ref > curator-version > upstream-pin > upstream-registration-ref (same-repo fallback) > branch-head (allow_head). All I/O is dependency-injected (ref_to_sha, canonical_full_name, version_range_resolver) so unit tests never touch the network. Strict-parser rejections are hoisted into the resolver's diagnostic stream as build errors, ready for the builder to surface as exit- code-2 failures. 21 new unit tests; 1035/1035 marketplace tests pass; lint pair silent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Wire UpstreamResolver into MarketplaceBuilder.build() so apm.yml packages declared with the upstream/plugin shape resolve to vanilla plugin entries in the emitted marketplace.json. Changes - builder.resolve() partitions the union of PackageEntry into direct vs. upstream entries, drives upstream resolution via a per-host RefResolver wiring (curator host reuses authenticated resolver; foreign hosts get unauthenticated resolver), and lifts upstream diagnostics into the BuildReport. - compose_marketplace_json gains an upstream_resolved kwarg and emits upstream plugins after direct ones (no interleaving in v1). Curator overrides win for description/version/tags; author/license/ repository/homepage stay curator-only -- matching the direct-package contract. - BuildDiagnostic gains an optional code field; level enum extended with 'error'. - ResolveResult and BuildReport carry upstream_entries + upstream_diagnostics. - build() round-trip-validates output via the lenient parse_marketplace_json (asserts plugin count) and fail-closes BEFORE composing/writing when upstream resolution emits a level=error diagnostic. Direct continue_on_error semantics are preserved. Tests - 7 new builder-upstream integration tests cover mixed-shape emission, upstream-only builds, no metadata.apm.* injection, curator-override precedence, git-subdir source shape, round-trip parsing, and the fail-closed gate on resolution errors. - 1042/1042 marketplace tests green (1035 prior + 7 new). - Lint pair silent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Extend LockFile with an upstreams: section that captures, for every upstream-sourced package emitted to the curator's marketplace.json, the upstream marketplace coordinates (host/owner/repo/path/manifest SHA + canonical_full_name guard) and the per-plugin pin (resolved SHA + emitted display name + resolved source dict). Provenance lives in apm.lock.yaml only. marketplace.json stays Anthropic-conformant -- no metadata.apm.* keys are injected -- which preserves the hard rule documented in builder.py. Changes - deps/lockfile.py: new LockedUpstream and LockedUpstreamPlugin dataclasses with to_dict / from_dict serialisers; LockFile.upstreams field; YAML write emits a sorted, deterministic upstreams: block; YAML read restores it. - marketplace/builder.py: build() calls a new _write_upstream_lockfile helper after the atomic marketplace.json write. The helper loads the existing lockfile, replaces the upstreams: block from the freshly resolved upstream packages, and rewrites the file. Empty-upstream builds are no-ops (existing block preserved). Tests - 7 new tests cover dataclass round-trip, deterministic sorted plugin output, LockFile YAML round-trip, builder integration writes the upstream section with all required fields, and the no-metadata.apm.* invariant still holds. - 1147/1147 marketplace + lockfile tests pass. - Lint pair silent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds the curator-facing CLI surface for managing upstream marketplaces, backed by the upstream pipeline already wired through schema -> parser -> cache -> resolver -> builder -> lockfile. Editor (yml_editor.py): - add_upstream_entry / remove_upstream_entry / list_upstream_entries - alias regex (case-sensitive), repo regex, dangling-reference guard on remove (rejects when any package still references the alias) - round-trip ruamel preserves comments and whitespace CLI (commands/marketplace/upstream.py): - 'apm marketplace upstream add <repo> --alias --ref|--branch [--path] [--host] [--allow-head] [--no-verify]' - 'apm marketplace upstream list' - 'apm marketplace upstream remove <alias>' - mutually exclusive --ref / --branch; --branch requires --allow-head - mutable refs auto-resolved to SHA via ls-remote (offline-tolerant) - exit code 2 on validation / resolution errors Registration: - 'upstream' added to _authoring_commands group ordering - subgroup wired in commands/marketplace/__init__.py Tests: - tests/unit/marketplace/test_yml_editor_upstreams.py (15 tests) - tests/unit/commands/test_marketplace_upstream.py (10 tests) - 1107 marketplace + plugin + upstream tests green - Lint pair silent Refs: #1136 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
CLI: - 'apm marketplace package add --upstream <alias> --plugin <name>' now exposes a plugin from a registered upstream marketplace under a curator-chosen display name. Mutually exclusive with positional SOURCE and with --subdir. - New 'add_upstream_package_entry' helper in yml_editor: validates alias is registered, enforces (upstream, plugin) tuple uniqueness and cross-shape name uniqueness against existing direct entries. Docs: - New guide: docs/.../guides/marketplace-upstreams.md (concept, trust model table, schema, quick start, CLI reference, reproducibility, v1 scope contract). - enterprise/security.md: rewrites the 'no proxy, no mirror' claim to reflect curated-pass-through trust model + cross-link to upstreams guide. APM still does not re-host upstream content. - packages/apm-guide/.apm/skills/apm-usage/commands.md: 4 new rows for 'package add --upstream' and the 'upstream' subgroup. - packages/apm-guide/.apm/skills/apm-usage/package-authoring.md: adds the upstreams: block + upstream-sourced packages[] schema example. - CHANGELOG.md: Unreleased entry. Tests: - 5 new CLI tests covering --upstream xor SOURCE, neither, unknown alias (exit 2), --subdir rejection, and the happy path. - 1112 marketplace + plugin + upstream tests green. - Lint pair silent. Refs: #1136 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
#1136) After integrating upstream resolution into MarketplaceBuilder, the success/dry-run log line still counted only direct packages ('len(report.resolved)') and would print '0 package(s)' when the marketplace contained only upstream-sourced entries -- discovered during the local E2E against abhigyanpatwari/GitNexus. Sum direct + upstream resolved counts so the user sees the actual emitted plugin count. Refs: #1136 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Applies findings from local APM Review Panel pass:
Blocking (docs):
- Register new 'Marketplace Upstreams' guide in Starlight sidebar
(docs/astro.config.mjs); previously orphaned from nav.
- Fix mislabelled quick-start step: 'apm marketplace upstream list'
shows registered upstreams, not their plugin contents.
- Document 'apm marketplace upstream {add,list,remove}' subcommand
group and extend 'package add' with --upstream/--plugin/--name/
--allow-head in reference/cli-commands.md.
Functional bugs (auth + cli-logging):
- upstream.py CLI helpers (_verify_upstream_repo, _resolve_upstream_
ref_to_sha) now build RefResolver via AuthResolver.resolve(host,
org=owner) and pass owner/repo (not full URL) to list_remote_refs.
Prior code constructed RefResolver() with no host/token and double-
prefixed the URL through build_https_clone_url, breaking GHE/GHES
upstreams and any private upstream repo. Public github.com path
was unaffected.
- builder.py: drop hardcoded [x]/[!]/[i] prefixes from
BuildDiagnostic.message text; CommandLogger adds the symbol via the
rendering layer, so the prior code would double-prefix.
- builder.py: warning-level upstream resolver diagnostics are now
also lifted into report.warnings so the existing
pack._render_marketplace_result rendering path surfaces them.
Previously they only reached report.diagnostics, which no caller
consumed -- mutable-pin warnings were silently dropped.
Defence-in-depth (supply-chain):
- upstream_cache._fetch_marketplace_json: catch requests.HTTPError
explicitly and re-raise UpstreamCacheError with status + URL only,
using 'from None' to drop the response object from __cause__. Prior
chain retained exc.response.request.headers which carries the
Authorization: token <PAT> header, leaking the curator's token to
any logger that walked __cause__.
Test coverage:
- New tests/unit/commands/test_pack_upstream_count.py pins the
pack-count fix at pack.py:216 -- asserts 'Built marketplace.json
(N package(s))' and 'Would write...' messages count
len(resolved) + len(upstream_resolved) for empty, direct-only,
upstream-only, and mixed builds.
Validation: ruff check + format --check both silent; full unit suite
7546 passed, 1 warning, 29 subtests.
Deferred (low signal / scope creep): SHA-40 regex consolidation,
_build_upstream_resolver factory extraction, integration-with-fixtures
test for full pack flow, automated byte-identical reproducibility
test, list-cmd tree-item polish, remove-success symbol nit.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Apply remaining recommended + nit findings from APM Review Panel local pass on the marketplace-upstreams branch. All 8 panelists already had their blocking findings landed in 2da2d39; this commit lands the long tail. py-arch: - Extract _build_upstream_resolver 100+ line closure stack into _UpstreamResolverFactory holding host_resolvers cache + ref/version resolvers as instance state. _build_upstream_resolver becomes a thin assembly point; existing test patches still work. - Consolidate the 40-char SHA regex from 5 marketplace + plugin-CLI call sites into ref_resolver.FULL_SHA_RE (single source of truth). The deps/github_downloader_validation regex (7-40 hex, case-insensitive) is intentionally left alone -- different semantics. cli-logging: - upstream list per-entry rows render via tree_item() (no [i] prefix) to avoid the double-symbol look under the count line. - upstream remove success uses the default sparkles ([*]) symbol; [+] (check) reads as 'addition' in this codebase. supply-chain: - LockedUpstreamPlugin.resolved_sha typed as str | None; from_dict preserves None instead of silently coercing to ''. Required for pin_source='branch-head' (allow_head opt-in) round-trip. - Fix inverted comment at upstream_cache.put(): manifest is written before meta, and the comment now matches the atomicity contract (incomplete entry = manifest without meta = skipped on read). oss-growth + doc-writer: - Replace <40-char-sha-or-tag> placeholder with a real GitNexus SHA + 'how to get the current SHA' git ls-remote hint. - Rewrite CHANGELOG [Unreleased] entry as a one-line hook + bullets; link to rendered docs URL instead of a repo-relative file path. - Reword 'warned' to 'requires explicit --allow-head opt-in' in the CLI reference table; --allow-head is a hard opt-in, not a warning. - Add 'Tag vs SHA: when to use which' subsection to the guide. - Add 'Failure modes' subsection with the three named errors curators will hit (unknown alias, repo-rename, moving branch without --allow-head). auth (nit defer with rationale): - Expand the canonical_full_name=None comment in _build_upstream_resolver to spell out the v1 trade-off: manifest-SHA round-trip already provides tamper detection; resolving the canonical name would require an authenticated GitHub REST call per upstream on every build, widening curator PAT scope. upstream_resolver.resolve_all docstring documents the sequential-only concurrency decision (fan-out is the per-upstream cache, manifest fetches dominate, threading hurts error attribution without measurable speedup at the current curator scale). test-coverage: - New CLI-layer duplicate-alias test (TestUpstreamAdd::test_duplicate_alias_exits_2): re-adding an existing alias hard-fails at exit code 2 and leaves the original entry untouched. - New byte-identical reproducibility regression trap (test_byte_identical_rebuild_produces_same_output): two consecutive builds against the same mixed apm.yml produce identical bytes. - New regression trap on direct-package emission (test_upstream_does_not_mutate_direct_package_emission): adding an upstream entry must not change the direct subset's emitted JSON. Validation: - ruff check + format --check both silent. - pytest tests/unit -q: 7549 passed, 1 warning, 29 subtests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Wave C (Recommended):
- C1: add --yes/-y flag to 'upstream remove' with pre-validation before prompt
- C2: require explicit --plugin when --upstream is set (remove silent default)
- C3: pack success line shows 'N direct + M upstream' breakdown when upstreams > 0
- C4: consolidate _REMOTE_SOURCE_RE into OWNER_REPO_RE in ref_resolver.py;
import in upstream_cache, upstream_parser, and yml_schema (DRY)
- C5: add content_sha256 to UpstreamCache meta.json; verify on cache hit
- C6-C8: docs -- move trust model to bottom of marketplace-upstreams.md,
add :::note[Planned] callouts for refresh and apm doctor,
replace live SHA with fictitious placeholder,
add cross-link from marketplace-authoring.md to upstreams guide
Wave D (Nits):
- D1: replace tree_item() with logger.info() for flat list rows
- D2: fix (s) pluralisation -- '1 upstream' / '2 upstreams'
- D3: remove unused List/Optional imports from yml_editor.py
- D4: nav label 'Marketplace Upstreams' -> 'Marketplace upstreams' (sentence-case)
- D5: scrub str(exc) -> type(exc).__name__ in upstream_cache.py log messages
Test updates: fix TestUpstreamRemove tests to pass --yes, update alias to
-bad-alias (digit-leading now valid per B5), update pack count test assertions
to match new 'N direct + M upstream' format.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ild pipeline (A2) Adds three end-to-end fixture tests for the upstream build pipeline without network calls: - test_mixed_build_emits_both_entries: verifies a direct+upstream apm.yml produces marketplace.json with both plugin entries and no metadata.apm.* keys (Anthropic-conformant pass-through contract). - test_lockfile_records_upstream_provenance: verifies apm.lock.yaml records manifest_sha and resolved plugin sha for the upstream alias. - test_offline_rebuild_is_byte_identical: verifies that an offline rebuild using the populated lockfile produces byte-identical output (reproducibility invariant). Both the direct package (pinned SHA ref) and upstream manifest (pre-populated UpstreamCache from fixture dict) bypass the network entirely, making these tests fast and deterministic. Also registers the new test file in scripts/test-integration.sh so it runs in CI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sergio-sisternes-epam
force-pushed
the
feature/marketplace-upstreams-1136
branch
from
dbba333 to
3394398
Compare
Description
Adds marketplace upstreams -- a curator-side feature that lets an APM
marketplace selectively re-expose plugins from external Claude Code
marketplaces (e.g. third-party
marketplace.jsonrepos) under anallow-list governance model with immutable commit pinning, while keeping
the emitted
marketplace.jsonstrictly Anthropic-conformant (no APMextensions in source objects).
The curator controls which external repos and which plugins are
visible. Consumers see the curator's marketplace as the single source of
truth and continue to fetch plugin content from the original git host
(no rehosting in v1).
Why
Internal / private marketplaces (e.g. enterprise curators) need to
re-expose useful upstream plugins under a vetted name without
maintaining forks. The existing model required the curator to fork or
re-author every plugin. Upstreams provide a thinner pass-through with:
referenced; unknown aliases fail closed at pack time.
HEAD-floats are rejected unless
--allow-headis set explicitly perupstream.
canonical_full_name;silent rename of an upstream repo fails the next pack.
apm packruns producebyte-identical
marketplace.jsonfrom the lockfile.upstream host, so the curator does not become a CDN.
Fixes #1136
What changed
New CLI surface under
apm marketplace:apm marketplace upstream add <owner/repo> --alias <name> --ref <sha>apm marketplace upstream listapm marketplace upstream remove <alias>apm marketplace package add --upstream <alias> --plugin <name> [--name <emit-as>](the
SOURCEargument and--upstreamare mutually exclusive)New schema (apm.yml
marketplace.upstreams:):New lockfile section (
apm.lock.yaml.upstreams:):records
host,owner,repo,manifest_sha,canonical_full_name,and per-emitted-plugin
{emitted_as, resolved_sha, resolved_source}for full provenance and rename-guard.
resolved_shais typedstr | Noneso thepin_source: branch-head(allow-head opt-in)round-trip preserves "unpinned" faithfully.
Builder integration:
MarketplaceBuildernow emits a singlemarketplace.jsoncombining direct + upstream packages; upstreamsource objects are pass-through with
path,ref,shaset. Theupstream-resolver wiring is encapsulated in a dedicated
_UpstreamResolverFactory(replacing a 100+ line nested-closurestack in the original draft).
Bug fix:
apm packpreviously printedBuilt marketplace.json (0 package(s))when only upstream packageswere present (
len(report.resolved)excludedreport.upstream_resolved).Counts now sum both tuples.
Trust model (v1)
Consumer-side custody is reserved for v2 (
distribution: rehost).Type of change
apm packcount)marketplace upstreams)extraction, lockfile typing)
Testing
uv run --extra dev pytest tests/unit -q-> 7549 passed, 1 warning, 29 subtests)
unit tests across 8 files; see breakdown below)
ruff check src/ tests/,ruff format --check src/ tests/)tests/integration/E2E for marketplace upstreams(deferred -- see "Out of scope" below for rationale; manual
local E2E in this PR substitutes until then)
Unit coverage by layer
The upstream feature is layered: schema -> parser -> cache ->
resolver -> builder -> lockfile -> editor -> CLI. Each layer has a
dedicated test module. Counts below are upstream-focused tests
introduced by this PR.
test_yml_schema_upstreams.pymarketplace.upstreams[]accepts ref orallow_head: true; rejects path traversal, invalid alias / repo / host, conflictingref+allow_head, duplicate aliases.test_upstream_parser.pymarketplace.json: required keys, plugin shape, source-object shape (git-subdironly in v1), refusal of APM extensions, non-utf8 / oversized / malformed input, fail-closed on unknown plugin types.test_upstream_cache.py(host, owner, repo, sha): hit / miss, atomic write order (manifest first, meta last -- partial entries skipped on read), TOCTOU rename, stale-meta detection, eviction safety, integrity on read.test_upstream_resolver.pyallow_head; rejects HEAD-float without opt-in; surfaces named errors (UpstreamUnknownAlias,UpstreamRepoRenamed,UpstreamHeadNotAllowed); host-resolver caching; canonical-name guard.test_builder_upstream_integration.pymarketplace.json, populates lockfileupstreams:block, emitsgit-subdirsource withpath+ref+sha. Includes (new this round) a byte-identical reproducibility trap and a "direct emission must not be perturbed by upstream additions" trap.test_lockfile_upstreams.pyapm.lock.yamlround-trip forupstreams:block:manifest_sha,canonical_full_name, per-plugin{emitted_as, resolved_sha, resolved_source};pin_source: branch-headround-tripsresolved_sha=Nonefaithfully.test_yml_editor_upstreams.pyapm.yml: alias add / remove preserves comments and key ordering; duplicate-alias is rejected at the editor layer;--allow-headand--refmutual exclusion.test_marketplace_upstream.pyupstream add / list / removeandpackage add --upstream: success paths, mutual-exclusion errors, duplicate-alias hard-fails at exit code 2 (regression trap added this round),listrendering, missing-alias errors.Builder-layer integration coverage
test_builder_upstream_integration.pyexercises the builder withreal ref-resolver / upstream-resolver / lockfile-writer wiring (the
git network calls are the only thing stubbed, via a
MagicMockfetch_callback). It is the closest thing to an integration testwe have for this feature and covers:
apm.yml(direct + upstream entries) -> single emittedmarketplace.jsonwith both subsets present.git-subdirwithpath,ref,shaallset on every upstream entry; no APM extensions leak through.
upstreams:block correctly populated withmanifest_shaandcanonical_full_name; per-pluginresolved_shamatches the upstreammarketplace.jsonsource.--allow-headopt-in path:pin_source: branch-head,resolved_sha=None, no SHA in emitted source object.MarketplaceBuilder.build()over the same inputs producesbyte-identical output.
mixed apm.yml must not change the direct subset's emitted JSON
byte-for-byte.
Local E2E (real upstream, manual)
Executed against the public repo
abhigyanpatwari/GitNexus(a realClaude Code marketplace) at a pinned SHA. This is a manual
end-to-end run; it is not wired into
tests/integration/orscripts/test-integration.sh(see "Out of scope" below).Verified:
marketplace.jsonis Anthropic-conformant (nometadata.apm.*keys); source object isgit-subdirwithpath: gitnexus-claude-plugin,refandshaboth pinned.apm.lock.yaml.upstreams.gitnexuspopulated with fullprovenance:
manifest_sha,canonical_full_name,per-plugin
resolved_sha.apm packproduced byte-identicalmarketplace.json(reproducibility gate met against a real upstream, not just the
unit-level mock).
and a non-zero exit code, leaving lockfile and
marketplace.jsonunchanged):
marketplace.packages[]->
UpstreamUnknownAlias.repo:to a differentowner/repowhile thelockfile still records the old
canonical_full_name->
UpstreamRepoRenamed.--allow-head->
UpstreamHeadNotAllowed.Out of scope (deferred)
apm doctorupstream health check.apm.ymlpolicy.upstreams.*governance keys.scripts/test-integration.sh).apm installE2E from a real curator marketplace(requires the curator repo to be hosted; not picked autonomously).
Documentation
docs/src/content/docs/guides/marketplace-upstreams.md(new guide).Quick-start uses a real GitNexus SHA + a
git ls-remoterecipe;CLI reference table distinguishes SHA / tag / branch+
--allow-headrows; "Tag vs SHA: when to use which" subsection; new "Failure
modes" subsection covering the three named errors curators will hit
(unknown alias, repo-rename, moving branch without
--allow-head).docs/src/content/docs/enterprise/security.md(rewrote the"no proxy, no mirror" claim to reflect the curated pass-through
model and link to the new guide).
packages/apm-guide/.apm/skills/apm-usage/commands.md(4 new rows).packages/apm-guide/.apm/skills/apm-usage/package-authoring.md(
upstreams:schema example).CHANGELOG.md[Unreleased]entry rewritten as a one-line hook +bullets, linking to the rendered docs URL
(
https://microsoft.github.io/apm/guides/marketplace-upstreams/).Review history
This branch went through a local APM Review Panel pass before
opening. The first round (
2da2d39a) landed all blocking-tierfindings; the second round (
7153f90b) lands the long tail ofrecommended + nit findings: the
_UpstreamResolverFactoryextraction, the 6-site SHA-40 regex consolidation into
ref_resolver.FULL_SHA_RE, the lockfile typing fix, the CLI symbolnits, the doc additions called out above, and three new
regression-trap tests.